test/bin/c2cc_common.sh (1)

363-383: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Unchecked command results in configure_pod_mtu/restart_microshift_and_wait.

Neither the tee write in configure_pod_mtu nor the systemctl restart microshift in restart_microshift_and_wait check run_command_on_vm's return code. Failures will only surface later (if at all) via wait_for_greenboot_on_hosts/the bigmtu.robot MTU assertion, with a less specific error than at the point of failure. Existing helpers in this file (e.g. full_vm_name) use || return 1 for this purpose.

🩹 Proposed fix

 configure_pod_mtu() {
     local -r mtu=$1
     for host in host1 host2 host3; do
         run_command_on_vm "${host}" "sudo tee /etc/microshift/ovn.yaml > /dev/null <<EOF
 mtu: ${mtu}
-EOF"
+EOF" || { echo "${host}: failed to write ovn.yaml" >&2; return 1; }
     done
 }

 restart_microshift_and_wait() {
     local -r junit_label="${1:-bigmtu_greenboot}"
     for host in host1 host2 host3; do
-        run_command_on_vm "${host}" "sudo systemctl restart microshift"
+        run_command_on_vm "${host}" "sudo systemctl restart microshift" || { echo "${host}: failed to restart microshift" >&2; return 1; }
     done

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/bin/c2cc_common.sh` around lines 363 - 383, The `configure_pod_mtu` and
`restart_microshift_and_wait` helpers ignore failures from `run_command_on_vm`,
so add explicit return-code propagation for the `tee` write and the `systemctl
restart microshift` command. Use the same `|| return 1` pattern already used by
helpers like `full_vm_name` so these functions fail immediately at the point of
error rather than only surfacing later in `wait_for_greenboot_on_hosts` or
tunnel checks.

test/assets/c2cc/nettest-pod.yaml (1)

8-19: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Harden nettest-pod further per manifest guidelines.

Missing readOnlyRootFilesystem: true, container resource limits, and automountServiceAccountToken: false (this pod doesn't need API access). As per path instructions: "securityContext: runAsNonRoot, readOnlyRootFilesystem, allowPrivilegeEscalation: false", "Resource limits (cpu, memory) on every container", and "automountServiceAccountToken: false unless needed".

🔒 Proposed hardening

 spec:
   terminationGracePeriodSeconds: 0
+  automountServiceAccountToken: false
   containers:
   - name: nettest
     image: registry.access.redhat.com/ubi9/ubi:9.6
     command: ["sleep", "infinity"]
     securityContext:
       allowPrivilegeEscalation: false
+      readOnlyRootFilesystem: true
       capabilities:
         drop:
         - ALL
       runAsNonRoot: true
       runAsUser: 10001
       seccompProfile:
         type: RuntimeDefault
+    resources:
+      limits:
+        cpu: 100m
+        memory: 64Mi
+      requests:
+        cpu: 50m
+        memory: 32Mi

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/assets/c2cc/nettest-pod.yaml` around lines 8 - 19, The nettest pod spec
is missing required hardening settings; update the nettest container and pod
manifest to match the security guidelines. In the nettest container definition,
add readOnlyRootFilesystem: true alongside the existing securityContext
settings, and add cpu/memory resource limits for the container. Also set
automountServiceAccountToken: false at the pod level since this test pod does
not need API access.

Source: Path instructions

test/resources/ipsec.resource (1)

143-151: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Docstring claims EMSGSIZE is asserted, but the check only verifies absence of "OK".

The docstring says this keyword Asserts EMSGSIZE (Message too long), but Should Not Contain ${stdout} OK will also pass for any unrelated failure (pod not found, oc CLI error, python traceback for a different reason, transient exec failure). This decouples the assertion from the actual MTU-boundary condition it's meant to verify, making the test pass for the wrong reason.

Also note the near-duplicate Python one-liner between this keyword and Ping With DF Bit And Verify (Lines 130-141) — worth extracting into a shared variable/keyword to avoid divergence if the socket options ever need adjusting.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/resources/ipsec.resource` around lines 143 - 151, The `Ping With DF Bit
Should Fail` keyword currently only checks that `${stdout}` does not contain
"OK", so it can pass for unrelated errors instead of proving EMSGSIZE; update
the assertion to verify the expected "Message too long"/EMSGSIZE failure from
the `Oc On Cluster`/`oc exec` command path. Keep the check tied to the actual
DF-bit UDP send behavior in this keyword, and consider extracting the repeated
Python socket one-liner shared with `Ping With DF Bit And Verify` into a common
variable or helper keyword to avoid future drift.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

test/resources/ipsec.resource (1)

161-167: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Capture stderr for easier failure diagnosis.

Unlike the DF-bit keywords (which redirect 2>&1), this command doesn't capture curl/dd stderr into ${stdout}, so a failed Should Contain Hello from assertion won't show why (e.g., curl connection error, timeout).

♻️ Suggested tweak

     ${stdout}=    Oc On Cluster
     ...    ${alias}
-    ...    oc exec curl-pod -n ${NAMESPACES}[${alias}] -- sh -c 'dd if=/dev/zero bs=${size} count=1 2>/dev/null | curl -sS --max-time 15 --data-binary `@-` http://${ip}:8080/cgi-bin/hello'
+    ...    oc exec curl-pod -n ${NAMESPACES}[${alias}] -- sh -c 'dd if=/dev/zero bs=${size} count=1 2>/dev/null | curl -sS --max-time 15 --data-binary `@-` http://${ip}:8080/cgi-bin/hello' 2>&1

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/resources/ipsec.resource` around lines 161 - 167, The Send Large Payload
And Verify keyword currently does not capture curl/dd stderr, so failures in the
Oc On Cluster command are hard to diagnose. Update the shell command inside this
keyword to redirect stderr into stdout the same way the DF-bit keywords do, so
${stdout} includes curl or dd error details before the Should Contain assertion
runs. Keep the change localized to Send Large Payload And Verify and preserve
the existing behavior otherwise.

test/suites/c2cc/ipsec/mtu.robot (2)

101-112: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

Fragile default-interface detection.

ip route show default | awk '{print $5}' | head -1 assumes exactly one relevant default route; with dual-stack or ECMP routes, head -1 may pick an interface that isn't the one actually carrying traffic, silently mis-targeting the MTU change.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/suites/c2cc/ipsec/mtu.robot` around lines 101 - 112, The
default-interface lookup in Configure Jumbo MTU On All Clusters is too fragile
because it relies on a single routed result and head -1. Update the Command On
Cluster command used to determine ${iface} so it selects the actual egress
interface more robustly, and keep the rest of the MTU change/verification flow
unchanged. Use the Configure Jumbo MTU On All Clusters keyword and the ${iface}
assignment as the main place to fix this.

---

60-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff

Stateful setup disguised as a test case.

Reconfigure Pod MTU To 8900 mutates cluster infra (config, restart, redeploy) as a regular test case rather than a [Setup]/suite fixture. If this test is filtered out, skipped, or fails partway, later Phase 3 tests (Verify Pod MTU Configuration, Jumbo MTU At Reduced Pod MTU, etc.) will silently execute against an undefined MTU state and produce misleading pass/fail results, with no repair path in Teardown. The suite comment acknowledges TEST_RANDOMIZATION=none as the safeguard, but partial failure of this one test case is not handled by the current teardown/setup structure.

Consider moving this into the suite structure as an explicit [Setup] for a nested test block, or ensure downstream tests defensively re-verify pod MTU rather than assuming success of a prior sibling test case.

Also applies to: 86-90

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/suites/c2cc/ipsec/mtu.robot` around lines 60 - 66, The MTU
reconfiguration step is currently implemented as a regular test case in
Reconfigure Pod MTU To 8900, which makes later Phase 3 tests depend on a sibling
test’s success. Move the cluster mutation logic into suite-level setup for the
Phase 3 block, or into a dedicated nested [Setup] fixture, so Configure Pod MTU
On All Clusters, Restart MicroShift On All Clusters, and Redeploy Test Workloads
always run before Verify Pod MTU Configuration and Jumbo MTU At Reduced Pod MTU.
If you keep the test case, add defensive re-verification in the downstream
checks so they do not assume the prior state transition succeeded.

Source: Learnings